Conversation
Consolidated duplicate _parse_response() and _build_prompt() methods from OllamaClassifier and GroqClassifier into the BaseClassifier, eliminating ~65 lines of duplicate code. Changes: - Moved _build_prompt() to BaseClassifier with comprehensive docstring - Handles prompt template formatting with character/player context - Subclasses can override for custom prompt building - Moved _parse_response() to BaseClassifier with comprehensive docstring - Parses Dutch-format LLM responses (Classificatie/Reden/Vertrouwen/Personage) - Handles Classification enum, confidence clamping, and error cases - Subclasses can override for different response formats - Added preflight_check() to GroqClassifier - Validates API key configuration - Tests API connectivity - Matches OllamaClassifier pattern Benefits: - DRY principle: Single implementation instead of duplicate code - Maintainability: Bug fixes and improvements apply to all classifiers - Extensibility: New classifiers can inherit default implementations - Type safety: Uses Classification enum from constants module Testing: - All 37 existing tests pass - Code coverage: 78% - No breaking changes - backward compatible Dependencies: - Uses Classification enum from src/constants.py (Refactor #7) Refs: Refactor #2+#3 - Consolidate Classifier Methods
Summary of ChangesHello @Gambitnl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the classifier architecture by consolidating shared prompt building and response parsing logic into the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the classifier classes by moving duplicated _build_prompt and _parse_response methods into the BaseClassifier, which improves maintainability and adheres to the DRY principle. The addition of a preflight_check to GroqClassifier is also a good improvement for consistency and robustness.
My review includes two main points on the new _parse_response method. First, a high-severity concern about the brittleness of the line-by-line parsing logic, which could fail with slightly varied LLM outputs. Second, a medium-severity suggestion to add logging for parsing errors on the confidence value to improve debuggability, instead of failing silently.
| lines = response.strip().split('\n') | ||
| for line in lines: | ||
| line = line.strip() | ||
| if line.startswith("Classificatie:"): | ||
| class_text = line.split(":", 1)[1].strip().upper() | ||
| try: | ||
| classification = Classification(class_text) | ||
| except ValueError: | ||
| self.logger.warning( | ||
| "Invalid classification '%s' for segment %s, defaulting to IC", | ||
| class_text, | ||
| index | ||
| ) | ||
| classification = Classification.IN_CHARACTER | ||
| elif line.startswith("Reden:"): | ||
| reasoning = line.split(":", 1)[1].strip() | ||
| elif line.startswith("Vertrouwen:"): | ||
| try: | ||
| conf_text = line.split(":", 1)[1].strip() | ||
| confidence = float(conf_text) | ||
| confidence = ConfidenceDefaults.clamp(confidence) | ||
| except ValueError: | ||
| pass | ||
| elif line.startswith("Personage:"): | ||
| char_text = line.split(":", 1)[1].strip() | ||
| if char_text.upper() != "N/A": | ||
| character = char_text |
There was a problem hiding this comment.
The current line-by-line parsing approach is brittle and may fail on slightly deviant LLM outputs. For instance:
- If the reasoning (
Reden) is multi-line, any line after the first that starts with a keyword (e.g.,Vertrouwen:) will be misinterpreted, truncating the reasoning and potentially corrupting another field. - The
elifstructure correctly handles out-of-order fields, but the multi-line issue is significant as it can lead to silent data corruption.
Consider a more robust parsing strategy. One option is to use a dictionary to first collect all key-value pairs, which would be more resilient to ordering and would make it easier to handle multi-line values for specific keys like Reden.
| confidence = float(conf_text) | ||
| confidence = ConfidenceDefaults.clamp(confidence) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Silently passing on a ValueError here can hide issues with the LLM's output for the Vertrouwen field. It's better to log a warning, which provides visibility for debugging while still allowing the fallback to the default confidence value. This would also make it consistent with how invalid Classificatie values are handled.
| pass | |
| self.logger.warning( | |
| "Invalid confidence value '%s' for segment %s, using default.", | |
| conf_text, | |
| index | |
| ) |
Addressed code review feedback from gemini-code-assist: 1. Fixed high-severity multi-line parsing issue: - Replaced brittle line-by-line parsing with regex-based extraction - Used re.DOTALL flag for reasoning field to handle multi-line values - Prevents corruption when field keywords appear in multi-line text - Handles out-of-order fields gracefully 2. Fixed medium-severity silent error handling: - Added warning log for invalid confidence values - Now consistent with classification error handling - Improves debuggability by surfacing parsing issues Changes: - _parse_response() now uses regex patterns for each field - Reden (reasoning) captures everything until next field - More resilient to LLM output variations - Better error visibility for debugging Testing: - All 37 tests still passing - Backward compatible with existing test cases Co-authored-by: gemini-code-assist[bot]
Resolved conflicts by keeping improved implementation: - Regex-based parsing (more robust for multi-line reasoning) - Enhanced error logging for invalid confidence values - Better documentation with override guidance Conflicts resolved in src/classifier.py: - Kept HEAD version with superior parsing strategy - Maintained code review improvements - All 37 tests passing Changes from main: - src/pipeline.py updates merged successfully
2 participants
Consolidated duplicate _parse_response() and _build_prompt() methods from OllamaClassifier and GroqClassifier into the BaseClassifier, eliminating ~65 lines of duplicate code.
Changes:
Moved _build_prompt() to BaseClassifier with comprehensive docstring
Moved _parse_response() to BaseClassifier with comprehensive docstring
Added preflight_check() to GroqClassifier
Benefits:
Testing:
Dependencies:
Refs: Refactor #2+#3 - Consolidate Classifier Methods